- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 33.6k
http: fix http agent keep alive #43380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
          
     Merged
      
      
            nodejs-github-bot
  merged 1 commit into
  nodejs:main
from
theanarkh:fix_http_agent_keep_alive
  
      
      
   
  Jun 18, 2022 
      
    
                
     Merged
            
            http: fix http agent keep alive #43380
                    nodejs-github-bot
  merged 1 commit into
  nodejs:main
from
theanarkh:fix_http_agent_keep_alive
  
      
      
   
  Jun 18, 2022 
              
            Conversation
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
    | Review requested: 
 | 
565464c    to
    8eaff1e      
    Compare
  
    | This needs a test | 
99536b6    to
    95d6aad      
    Compare
  
    
              
                    mscdex
  
              
              reviewed
              
                  
                    Jun 11, 2022 
                  
              
              
            
            
95d6aad    to
    9ed9d4d      
    Compare
  
    
              
                    ShogunPanda
  
              
              approved these changes
              
                  
                    Jun 13, 2022 
                  
              
              
            
            
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
              
                    mcollina
  
              
              approved these changes
              
                  
                    Jun 13, 2022 
                  
              
              
            
            
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
              
                    jasnell
  
              
              reviewed
              
                  
                    Jun 13, 2022 
                  
              
              
            
            
              
                    jasnell
  
              
              reviewed
              
                  
                    Jun 13, 2022 
                  
              
              
            
            
9ed9d4d    to
    0d1ad29      
    Compare
  
    
      
        
      
      
  
    16 tasks
  
0d1ad29    to
    aeadf39      
    Compare
  
    
              
                    rickyes
  
              
              approved these changes
              
                  
                    Jun 14, 2022 
                  
              
              
            
            
      
        
      
      
  
    17 tasks
  
      
        
      
      
  
    22 tasks
  
| @ShogunPanda Hi, can you help trigger CI again ? Thanks ! | 
              
                    trivikr
  
              
              approved these changes
              
                  
                    Jun 16, 2022 
                  
              
              
            
            
  This was referenced Jun 17, 2022 
      
| Landed in fe776b8 | 
      
        
      
      
  
    30 tasks
  
    
  targos 
      pushed a commit
      that referenced
      this pull request
    
      Jul 12, 2022 
    
    
      
  
    
      
    
  
PR-URL: #43380 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
    
  targos 
      pushed a commit
      that referenced
      this pull request
    
      Jul 18, 2022 
    
    
      
  
    
      
    
  
PR-URL: #43380 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
    
  targos 
      pushed a commit
      that referenced
      this pull request
    
      Jul 31, 2022 
    
    
      
  
    
      
    
  
PR-URL: #43380 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
    
  guangwong 
      pushed a commit
        to noslate-project/node
      that referenced
      this pull request
    
      Oct 10, 2022 
    
    
      
  
    
      
    
  
PR-URL: nodejs/node#43380 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ricky Zhou <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment
  
      Labels
      
    author ready
  PRs that have at least one approval, no pending requests for changes, and a CI started. 
  
    commit-queue-squash
  Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 
  
    http
  Issues or PRs related to the http subsystem. 
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
When
options.keepAliveincreateSocketfunction is true it leads to a bug. becausecreateSocketwill callthis.createConnection(options, oncreate)which will create a socket and set two fields in socket.this[kSetKeepAliveInitialDelay]will be0becauseoptions.keepAliveInitialDelayis undefined. When the connection is finished,afterConnectwill be called and use this two fields, the related code is as follow.It calls
setKeepAlivewith 0 (self[kSetKeepAliveInitialDelay]).Then when the
freeevent of agent is emitted, agent will callsetKeepAliveinkeepSocketAlive, the code is as follow.enable !== this[kSetKeepAlive]will return false, so the agent do nothing which lead to a bug.Currently http agent only set keepalive on some sockets (when
freeevent is emitted). Maybe we can set keepalive for all sockets ? otherwise i think we should delete thekeepAlivefield of options before callthis.createConnectionincreateSocket.Refs: #41965.
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected subsystem: http